Add makeflag for building TBB under UCRT automatically #2948

andrjohns · 2023-09-21T20:31:15Z

Summary

Windows users (and cmdstan interfaces) currently need to add -D_UCRT to make/local when building on windows with a UCRT toolchain. This PR updates the makefiles to detect whether a UCRT toolchain is being used on Windows, and to add the flag automatically if needed.

We can detect UCRT-ness by checking whether the UCRT macro is defined after pre-processing the windows.h header

UCRT Toolchain:

PS C:\> echo "#include <windows.h>"| C:/rtools43/ucrt64/bin/g++ -E -dM -  | grep -i _UCRT
#define _UCRT

Non-UCRT Toolchain:

PS C:\> echo "#include <windows.h>"| C:/rtools43/mingw64/bin/g++ -E -dM -  | grep -i _UCRT
PS C:\>

I've tested that the TBB (and Stan) successfully build under both the UCRT mingw-w64-ucrt-x86_64-gcc and the non-UCRT mingw-w64-x86_64-gcc toolchains

Tests

N/A

Side Effects

N/A

Release notes

Automatically detect UCRT toolchain use on Windows

Checklist

Math issue #(issue number)
Copyright holder: Andrew Johnson

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- dependencies checks pass, (make test-math-dependencies)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

WardBrian · 2023-09-21T20:35:23Z

Isn't windows.h famously huge? How long does that command take to run?

Do we currently use grep on Windows?

andrjohns · 2023-09-21T20:54:06Z

Sigh, the "works on my machine" strikes again. Will debug and update

SteveBronder

This looks good but imo it may be nice to only run that grep command if we are using windows. About how long does it take? I don't have a windows computer so I can't test it locally. Also can you use the -m 1 flag with grep so that it just stops on the first match?

WardBrian · 2023-09-26T19:52:54Z

I am opposed to calling grep on Windows at all - I'm not sure if it is bundled in rtools, but the standard mingw compiler distribution wouldn't have it by default AFAIK

Compiling windows.h and grepping on every single invocation also seems like a bad idea, so if we could have something that persists this to disk somehow that would be great

SteveBronder · 2023-09-26T20:16:35Z

I am opposed to calling grep on Windows at all - I'm not sure if it is bundled in rtools, but the standard mingw compiler distribution wouldn't have it by default AFAIK

Is it findstr on windows?

Compiling windows.h and grepping on every single invocation also seems like a bad idea, so if we could have something that persists this to disk somehow that would be great

Let's wait to see how long it takes from @andrjohns. Finding the first match of _UCRT is different then grepping all of windows.h

WardBrian · 2023-09-26T20:17:59Z

This is grepping on the result of the compiler pre-processing windows.h, not just grepping the file itself

andrjohns · 2023-09-26T21:18:07Z

I am opposed to calling grep on Windows at all - I'm not sure if it is bundled in rtools, but the standard mingw compiler distribution wouldn't have it by default AFAIK

Is it findstr on windows?

Good call! findstr seems to work just the same

Compiling windows.h and grepping on every single invocation also seems like a bad idea

It takes about a second for me, and this is running on an Windows ARM VM on an M1 Mac, with g++ running through the windows arm-to-x64 translation-layer:

PS C:\Users\andrew\Desktop\math> Measure-Command { echo "#include <windows.h>" | g++ -E -dM -  | findstr -i _UCRT }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 1
Milliseconds      : 300
Ticks             : 13004542
TotalDays         : 1.50515532407407E-05
TotalHours        : 0.000361237277777778
TotalMinutes      : 0.0216742366666667
TotalSeconds      : 1.3004542
TotalMilliseconds : 1300.4542

andrjohns · 2023-09-26T21:21:27Z

And it looks like the Powershell Select-String function is slightly faster:

PS C:\Users\andrew\Desktop\math> Measure-Command { echo "#include <windows.h>" | g++ -E -dM -  | Select-String _UCRT }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 838
Ticks             : 8387663
TotalDays         : 9.70794328703704E-06
TotalHours        : 0.000232990638888889
TotalMinutes      : 0.0139794383333333
TotalSeconds      : 0.8387663
TotalMilliseconds : 838.7663

WardBrian · 2023-09-26T21:24:37Z

I think sticking to cmd.exe programs is probably the best for compatibility. Findstr might also be faster if you use /l to disable regex searching

andrjohns · 2023-09-26T21:28:54Z

I think sticking to cmd.exe programs is probably the best for compatibility. Findstr might also be faster if you use /l to disable regex searching

Not much benefit unfortunately:

PS C:\Users\andrew\Desktop\math> Measure-Command { echo "#include <windows.h>" | g++ -E -dM -  | findstr /l _UCRT }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 1
Milliseconds      : 241
Ticks             : 12413473
TotalDays         : 1.43674456018519E-05
TotalHours        : 0.000344818694444444
TotalMinutes      : 0.0206891216666667
TotalSeconds      : 1.2413473
TotalMilliseconds : 1241.3473

andrjohns · 2023-09-26T21:35:44Z

I just did a GHA actions run using the windows-latest runner to check the timing, and it's ~750ms for native windows: https://github.com/stan-dev/math/actions/runs/6318530408/job/17157709773

Result:


Run Measure-Command { echo "#include <windows.h>" | g++ -E -dM -  | findstr /l _UCRT }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 14
Milliseconds      : 751
Ticks             : 147510291
TotalDays         : 0.000170729503472222
TotalHours        : 0.00409750808333333
TotalMinutes      : 0.245850485
TotalSeconds      : 14.7510291
TotalMilliseconds : 14751.0291

andrjohns · 2023-09-26T21:40:13Z

Oh wait hold on, that's 14 seconds and 750 ms. Sheesh never mind

andrjohns · 2023-09-26T21:50:31Z

Back to the drawing board!

WardBrian · 2023-09-26T21:54:47Z

I think it would be fine if this was saved to a file somewhere (like make/os?) as long as it was deleted by “make clean”. 14s isn’t a killer if it only happens during “make build”

andrjohns · 2023-09-26T22:17:58Z

We might be able to skip that completely, I started chasing through the includes in windows.h to see how/where _UCRT would get defined, and it looks like MinGW uses this macro:

#if !defined(_UCRT) && ((__MSVCRT_VERSION__ >= 0x1400) || (__MSVCRT_VERSION__ >= 0xE00 && __MSVCRT_VERSION__ < 0x1000))
/* Allow both 0x1400 and 0xE00 to identify UCRT */
#define _UCRT
#endif

WardBrian · 2023-09-27T19:47:48Z

make/compiler_flags

+  ifeq (,$(wildcard $(MATH)make/ucrt))
+    UCRT_STRING := $(shell echo "#include <windows.h>" | $(CXX) -E -dM -  | findstr _UCRT)
+    ifneq (,$(UCRT_STRING))
+      IS_UCRT ?= true
+    else
+      IS_UCRT ?= false
+    endif
+    $(shell echo "IS_UCRT ?= $(IS_UCRT)" > $(MATH)make/ucrt)
+  else
+    -include $(MATH)make/ucrt
+  endif


I think you can make this more concise by defining a make rule who's result is make/ucrt, which will then be built when you include it. This is how the .d files for dependencies work

Oh cool, TIL

Tried out the make rule, but I'm not sure about the best place to put the rule. Thoughts/prefs?

I think putting it just above where it is included is probably fine? I think you can put rules in places that are conditionally defined

andrjohns · 2023-09-27T19:47:57Z

We might be able to skip that completely, I started chasing through the includes in windows.h to see how/where _UCRT would get defined, and it looks like MinGW uses this macro

Unfortunately we can't use that Makefile logic since MinGW has their own logic for setting MSVCRT_VERSION, which I'd rather not hardcode. I've instead gone back to the windows.h approach but with caching

WardBrian · 2023-09-27T19:49:06Z

This might be a silly question but does windows.h always exist (e.g., on a system without MSVC etc installed)

andrjohns · 2023-09-27T20:07:01Z

This might be a silly question but does windows.h always exist (e.g., on a system without MSVC etc installed)

It will also exist if MinGW is installed (they provide a version as well). But if the system needs UCRT then I'd say it's pretty rare they wouldn't have windows.h

andrjohns · 2023-09-28T06:08:54Z

Looks like all passes (hooray!). Should we ask another dev/user with a windows machine to test the basic setup locally:

mingw32-make clean-all
mingw32-make -f make/standalone math-libs -j2

Just to make sure the initial caching process isn't too obnoxious for the dev workflow (or initial cmdstan build)

WardBrian

Two small comments. I will try this branch on my Windows machine tomorrow before final approval.

Thanks @andrjohns

WardBrian · 2023-09-28T14:30:57Z

make/tests

@@ -106,7 +106,7 @@ else
 endif

 %.hpp-test : %.hpp test/dummy.cpp
-	$(COMPILE.cpp) $(CXXFLAGS) -O0 -include $^ -o $(DEV_NULL) -Wunused-local-typedefs
+	$(COMPILE.cpp) $(CXXFLAGS) -O0 -include $^ -S -o $(DEV_NULL) -Wunused-local-typedefs


I'm not sure where this rule is used, is there a reason for this change in this PR?

It's a MinGW UCRT-specific bug, where the toolchain doesn't recognise the nul device for some reason: msys2/MINGW-packages#10730

Otherwise it fails with:

Assembler messages: Fatal error: can't create nul: Invalid argument

But I should have included it only for Win+UCRT, so I've fixed that now

I can't replicate the failure locally, which makes me think it's related to either:

rtools43 uses gcc 13.2 while GHA uses 12.2

rtools43 gcc built by MSYS2, GHA by Mingw-W64

RTools43 g++:

g++.exe (Rev2, Built by MSYS2 project) 13.2.0

GHA g++:

g++.exe (x86_64-posix-seh-rev2, Built by MinGW-W64 project) 12.2.0

WardBrian · 2023-09-28T14:32:00Z

makefile

@@ -125,6 +125,7 @@ clean-deps:
 	@$(RM) $(call findfiles,test,*.d.*)
 	@$(RM) $(call findfiles,lib,*.d.*)
 	@$(RM) $(call findfiles,stan,*.dSYM)
+	@$(RM) $(call findfiles,make,ucrt)


Just noting so we remember to do it: it seems like stan and cmdstan do not call the clean target of their submodules, so we will need to manually add something which deletes this higher up.

WardBrian · 2023-09-28T18:38:52Z

It looks like the Windows Header Checks are failing in #2952 and #2950, so we can definitely conclude this PR is doing something right by fixing them here

WardBrian

Tried with both rtools43 (UCRT) and rtools40 on this branch, both work. The caching makes it so the build time is essentially no different

Thanks @andrjohns

Add makeflag for building TBB under UCRT automatically

133e1cd

andrjohns requested a review from WardBrian September 21, 2023 20:31

andrjohns added 3 commits September 22, 2023 10:07

Escape character needed

a238c48

Fixup header tests

2e3ab3b

Weird breakage

1ae8e5b

WardBrian mentioned this pull request Sep 23, 2023

Docs/701 pathfinder stan-dev/cmdstanpy#707

Merged

2 tasks

osflags

373c783

SteveBronder previously approved these changes Sep 26, 2023

View reviewed changes

andrjohns closed this Sep 26, 2023

Add caching and cleaning for UCRT check

55e59f7

andrjohns reopened this Sep 27, 2023

andrjohns dismissed SteveBronder’s stale review via 55e59f7 September 27, 2023 19:45

WardBrian reviewed Sep 27, 2023

View reviewed changes

Test using make rule

9ee98c9

andrjohns added 2 commits September 27, 2023 23:14

Move ucrt rule, add to gitignore

441415f

Test fix no longer needed?

cd604f0

WardBrian reviewed Sep 28, 2023

View reviewed changes

andrjohns added 2 commits September 29, 2023 11:44

Test fix still needed

061f69c

Only add flag for UCRT

dfe460d

WardBrian approved these changes Sep 29, 2023

View reviewed changes

WardBrian merged commit 598dba7 into develop Sep 29, 2023
8 checks passed

WardBrian mentioned this pull request Sep 29, 2023

Tracking: Changes in Stan 2.34 roualdes/bridgestan#173

Closed

andrjohns mentioned this pull request Oct 5, 2023

Manual UCRT config no longer needed on Windows stan-dev/docs#676

Merged

3 tasks

WardBrian mentioned this pull request Dec 19, 2023

undefined reference error after installing cmdstan with install_cmdstan --compiler on windows gh actions stan-dev/cmdstanpy#722

Closed

andrjohns mentioned this pull request May 4, 2024

Update tests to use rtools-provided make stan-dev/stan#3281

Merged

3 tasks

WardBrian deleted the tbb-ucrt-makeflags branch August 5, 2024 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add makeflag for building TBB under UCRT automatically #2948

Add makeflag for building TBB under UCRT automatically #2948

andrjohns commented Sep 21, 2023

WardBrian commented Sep 21, 2023

andrjohns commented Sep 21, 2023

SteveBronder left a comment

WardBrian commented Sep 26, 2023

SteveBronder commented Sep 26, 2023

WardBrian commented Sep 26, 2023

andrjohns commented Sep 26, 2023

andrjohns commented Sep 26, 2023

WardBrian commented Sep 26, 2023

andrjohns commented Sep 26, 2023

andrjohns commented Sep 26, 2023 •

edited

Loading

andrjohns commented Sep 26, 2023

andrjohns commented Sep 26, 2023

WardBrian commented Sep 26, 2023

andrjohns commented Sep 26, 2023

WardBrian Sep 27, 2023

andrjohns Sep 27, 2023

andrjohns Sep 27, 2023

WardBrian Sep 27, 2023

andrjohns commented Sep 27, 2023

WardBrian commented Sep 27, 2023

andrjohns commented Sep 27, 2023

andrjohns commented Sep 28, 2023

WardBrian left a comment

WardBrian Sep 28, 2023

andrjohns Sep 29, 2023

andrjohns Sep 29, 2023

WardBrian Sep 28, 2023

WardBrian commented Sep 28, 2023

WardBrian left a comment

Add makeflag for building TBB under UCRT automatically #2948

Add makeflag for building TBB under UCRT automatically #2948

Conversation

andrjohns commented Sep 21, 2023

Summary

Tests

Side Effects

Release notes

Checklist

WardBrian commented Sep 21, 2023

andrjohns commented Sep 21, 2023

SteveBronder left a comment

Choose a reason for hiding this comment

WardBrian commented Sep 26, 2023

SteveBronder commented Sep 26, 2023

WardBrian commented Sep 26, 2023

andrjohns commented Sep 26, 2023

andrjohns commented Sep 26, 2023

WardBrian commented Sep 26, 2023

andrjohns commented Sep 26, 2023

andrjohns commented Sep 26, 2023 • edited Loading

andrjohns commented Sep 26, 2023

andrjohns commented Sep 26, 2023

WardBrian commented Sep 26, 2023

andrjohns commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrjohns commented Sep 27, 2023

WardBrian commented Sep 27, 2023

andrjohns commented Sep 27, 2023

andrjohns commented Sep 28, 2023

WardBrian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WardBrian commented Sep 28, 2023

WardBrian left a comment

Choose a reason for hiding this comment

andrjohns commented Sep 26, 2023 •

edited

Loading